Fix inconsistency between grapheme_substr() and substr() #6163
Conversation
kocsismate
changed the title
|
The scope is much bigger than just grapheme_substr("", 0, ...); Please revisit all the changes as there are lots of inconsistencies and backward compatibility breaks due to the new exceptions! |
| if ( OUTSIDE_STRING(lstart, str_len)) { | ||
| if (str_len == 0 && lstart == 0) { | ||
| RETURN_EMPTY_STRING(); | ||
| } else if (OUTSIDE_STRING(lstart, str_len)) { |
There was a problem hiding this comment.
I think the real problem is in OUTSIDE_STRING, which should only consider lstart > str_len to be out-of-bounds, not lstart >= str_len. "One past the end" is generally always considered a valid offset (also in strpos etc).
Though this is just a "definitely wrong" check, as this is a grapheme cluster offset, the actual offset check happens later...
There was a problem hiding this comment.
I think even lstart > str_len should be allowed for grapheme_substr() (and probably other functions). Whether it should return "" or false in that case is up for debate but throwing an exception IMHO cripples the API and makes it inconsistent with substr/mb_substr.
There was a problem hiding this comment.
I'll look into this tonight
|
Closing this in favor of #6182, which now also implements the update to grapheme_substr(). |
grapheme_substr("", 0, $length)should return an empty string no matter what integer value$lengthhas. This hasn't been the case before, so let's make this behaviour consistent withsubstr().A remaining inconcistency is that while
grapheme_substr("abc", strlen("abc"), $length)throws an exception,substr("abc", strlen("abc"), $length)returns"", butsubstr("abc", strlen("abc") + 1, $length)returnsfalse. It seems to me thatsubstr()is even inconsistent with itself, but it's pretty much probable that I forgot about this change, and the reasoning behind this.